cabal-install: Be less eager to configure external programs#10731
cabal-install: Be less eager to configure external programs#10731mergify[bot] merged 3 commits intohaskell:masterfrom
Conversation
|
Hey @mpickering! Any chance you could take a brief look at #2015? I believe it's closely related to the issue here. In particular, I tried to look into it, and it seems to stem, as you say, from storing the compiler's configuration on disk, and configuration includes The code for |
|
@ulysses4ever I've just been assigned to look at these two regressions, but I will comment on the issue about how I would begin with that problem. |
ea2f922 to
48c8638
Compare
|
@sheaf and I discussed this and for the backport, the have gone with a revert of the original patch. I have added two new tests which check for regressions introduced in the original patch. In the future we will work on a proper fix the original issue the reverted patch attempted to fix. |
48c8638 to
4bbc0dd
Compare
ee2b3fb to
6141c49
Compare
|
This should be ready now if anyone wants to take a look? @ulysses4ever @jasagredo perhaps? |
|
Is there anyone available to provide the second review on this patch? |
jasagredo
left a comment
There was a problem hiding this comment.
It is unfortunate that we have to revert back to the wasteful re-computation of the step, but at least this fixes my issue. Thanks!
This reverts commit 8bdda9c. In configureCompiler the call to configureAllKnownPrograms was too eager. When called it selected the version of tools from PATH (such as alex), and then when a package was configured these tools were passed using `--with-alex` options to configure, which meant that the build-tool-depends versions were not used. (See haskell#10692) Why was this call introduced in the first place? Because configureCompiler would a different result depending on whether: * It is run for the first time, the `ProgramDb` will contain unconfigured programs. * It is run subsequently, `ProgramDb` is read from disk, it does not contain unconfigured programs. Reverting this commit rexposes the bug that the serialised ProgramDb will not contain UnconfiguredPrograms (in the case where reconfiguring is avoided). However, there are no code paths which require this logic in `cabal-install` currently. The configuration phase happens again each time that `Cabal` is called, with a populated `ProgramDb`. We will fix this before the next major `cabal-install` release, but it would not be suitable to backport. In the future we will fix this properly by refactoring `configureCompiler` so that `ProgramDb` is not serialised. The general approach will be to make `configCompilerEx` return a pair of configured programs (`ghc` and `ghc-pkg`) and expose an additional function from `Cabal` which uses these two programs to perform the modifications to the `ProgramDb` which `configCompilerEx` performs. Also see haskell#2238 and haskell#9840 Fixes haskell#10692
The testcase is not so easy to write because * The bug only surfaces when the build-tool you are depending on is known (ie alex, happy etc) * But then it is tricky to write a test, as we can't depend on the known tools or bundle the source for them. * So we create a fake "alex", which cabal then invokes on a fake ".x" file. This is maybe a bit fragile if the way cabal invokes alex changes in future, but then the test can be modified as well. Ticket haskell#10692
Whilst fixing haskell#10692, I realised there was also this bug where extra-prog-path would not be honoured for specific packages. The idea behind extra-prog-path is that each local package can use a different version of a preprocessor if desired.
|
@mergify backport 3.14 |
✅ Backports have been createdDetails
|
…rograms (#10875) * Revert "cabal-install configureCompiler: configure progdb" This reverts commit 8bdda9c. In configureCompiler the call to configureAllKnownPrograms was too eager. When called it selected the version of tools from PATH (such as alex), and then when a package was configured these tools were passed using `--with-alex` options to configure, which meant that the build-tool-depends versions were not used. (See #10692) Why was this call introduced in the first place? Because configureCompiler would a different result depending on whether: * It is run for the first time, the `ProgramDb` will contain unconfigured programs. * It is run subsequently, `ProgramDb` is read from disk, it does not contain unconfigured programs. Reverting this commit rexposes the bug that the serialised ProgramDb will not contain UnconfiguredPrograms (in the case where reconfiguring is avoided). However, there are no code paths which require this logic in `cabal-install` currently. The configuration phase happens again each time that `Cabal` is called, with a populated `ProgramDb`. We will fix this before the next major `cabal-install` release, but it would not be suitable to backport. In the future we will fix this properly by refactoring `configureCompiler` so that `ProgramDb` is not serialised. The general approach will be to make `configCompilerEx` return a pair of configured programs (`ghc` and `ghc-pkg`) and expose an additional function from `Cabal` which uses these two programs to perform the modifications to the `ProgramDb` which `configCompilerEx` performs. Also see #2238 and #9840 Fixes #10692 (cherry picked from commit 1c64bb8) # Conflicts: # cabal-testsuite/PackageTests/ExtraProgPath/setup.out * Add a test to check that build-tool-depends are used (#10692) The testcase is not so easy to write because * The bug only surfaces when the build-tool you are depending on is known (ie alex, happy etc) * But then it is tricky to write a test, as we can't depend on the known tools or bundle the source for them. * So we create a fake "alex", which cabal then invokes on a fake ".x" file. This is maybe a bit fragile if the way cabal invokes alex changes in future, but then the test can be modified as well. Ticket #10692 (cherry picked from commit 24f8395) * Add a test to check that extra-prog-path is honoured for local packages Whilst fixing #10692, I realised there was also this bug where extra-prog-path would not be honoured for specific packages. The idea behind extra-prog-path is that each local package can use a different version of a preprocessor if desired. (cherry picked from commit 2c19bf3) * fixup! fix conflict --------- Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com> Co-authored-by: Artem Pelenitsyn <a.pelenitsyn@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
In configureCompiler the call to configureAllKnownPrograms was too eager. When called it selected the version of tools from PATH (such as alex), and then when a package was configured these tools were passed using
--with-alexoptions to configure, which meant that the build-tool-depends versions were not used. (See #10692)Why was this call introduced in the first place? Because configureCompiler would a different result depending on whether:
ProgramDbwill contain unconfigured programs.ProgramDbis read from disk, it does not contain unconfigured programs.A surgical way to fix this is to avoid configuring the programs, and manually adding back the builtinPrograms to the ProgramDb, so the ProgramDb returned by configureCompiler always contains all the unconfigured programs.
The testcase is not so easy to write because
Also see #2241 and #9840
Fixes #10692
Please read Github PR Conventions and then fill in one of these two templates.
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.